Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove uses of FnBox #851

Closed

Conversation

KamilaBorowska
Copy link
Contributor

FnBox is unlikely to be ever stabilized, as unsized_locals fixes Box<dyn FnOnce()> to work.

FnBox is unlikely to be ever stabilized, as unsized_locals fixes
Box<dyn FnOnce()> to work.
Copy link
Collaborator

@jebrosen jebrosen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hesitant to make this change when it had just become possible, but it seems to have been around a while now and isn't going away. It also might be worth waiting for rust-lang/rust#55431 which should let us do this without needing #[feature(unsized_locals)] ourselves.

@@ -146,9 +145,10 @@ impl Fairing for AdHoc {
fn on_attach(&self, rocket: Rocket) -> Result<Rocket, Rocket> {
if let AdHocKind::Attach(ref mutex) = self.kind {
let mut option = mutex.lock().expect("AdHoc::Attach lock");
option.take()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra let binding should be unnecessary -- just replace the line .call_box((rocket,)) with (rocket).

Copy link
Contributor Author

@KamilaBorowska KamilaBorowska Dec 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unnecessary, however it looks ugly when having a function call in a multi-line chain like this.

@KamilaBorowska
Copy link
Contributor Author

KamilaBorowska commented Dec 9, 2018

Also, from what I understand, the plan in rustc is to deprecate and remove FnBox as soon as possible.

@SergioBenitez
Copy link
Member

@xfix Where is that happening? We should ideally ask for that to be postponed.

@KamilaBorowska
Copy link
Contributor Author

@jebrosen
Copy link
Collaborator

jebrosen commented Dec 9, 2018

The last I checked:

So I don't think this will happen too quickly and we can probably wait a bit longer. I think the only reason not to merge this would be that unsized_locals might end up not panning out and it would have to be reverted.

@KamilaBorowska
Copy link
Contributor Author

KamilaBorowska commented Dec 10, 2018

Even then, if unsized_locals ends up not panning out (which I don't see happening, I think it's sound enough), and even if it were to happen by this point there should be boxed_closure_impls feature which will make this work.

@jebrosen
Copy link
Collaborator

jebrosen commented Apr 5, 2019

With rust-lang/rust#59500 merged, this should be possible without the feature gate once the next nightly is released. That would require increasing the minimum rustc version, more so than this PR already does.

@jebrosen
Copy link
Collaborator

Merged in 7ab1c42.

@jebrosen jebrosen closed this Apr 13, 2019
@jebrosen jebrosen added the pr: merged This pull request was merged manually. label Apr 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: merged This pull request was merged manually.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants